-
Notifications
You must be signed in to change notification settings - Fork 162
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adjust MarkAsReadWidget text to be bolder, use Source Sans 3 #376
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @sirpengi! One small comment below.
lib/widgets/message_list.dart
Outdated
textStyle: const TextStyle( | ||
fontFamily: 'Source Sans 3', | ||
fontSize: 18, | ||
fontWeight: FontWeight.w400, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fontWeight: FontWeight.w400,
is redundant with .merge(weightVariableTextStyle(context))
.
a9d4111
to
531cae1
Compare
@chrisbobbe readjusted per your recommendations! |
531cae1
to
2329bc3
Compare
Readjusted button height to 38px based on https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/flutter.3A.20Mark-as-read/near/1680573 |
2329bc3
to
0cd3809
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing these! Comments below.
lib/widgets/message_list.dart
Outdated
padding: const EdgeInsets.all(10), | ||
textStyle: const TextStyle(fontSize: 18, fontWeight: FontWeight.w200), | ||
shape: RoundedRectangleBorder(borderRadius: BorderRadius.circular(5)), | ||
backgroundColor: const HSLColor.fromAHSL(1,227,0.78,0.59).toColor(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: space after comma
Also Chris observed in that chat thread that this is the same color as the unread markers on messages, and that seems like not a coincidence — those markers are what this button will eliminate, and the shared color draws the connection. So let's factor it out with a name. Can be a static on _UnreadMarker
, like static final color = …
.
lib/widgets/message_list.dart
Outdated
textStyle: const TextStyle(fontSize: 18, fontWeight: FontWeight.w200), | ||
shape: RoundedRectangleBorder(borderRadius: BorderRadius.circular(5)), | ||
backgroundColor: const HSLColor.fromAHSL(1,227,0.78,0.59).toColor(), | ||
fixedSize: const Size.fromHeight(38), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to have no effect.
Looking with the inspector:
- The button's height as a tap target is 48px. That's because Flutter's Material implementation is providing that behavior for us automatically, which is handy.
- Its height as it participates in layout is also 48px. That's fine but probably means we want to reduce the further padding we give it above and below.
- The height of the visible button area is 40px. That reflects the default for
ButtonStyle.minimumSize
forFilledButton
.
So we can override minimumSize
here to get the visible height to 38px. The 48px for the touchable height is good and we can leave that as it is. But then the surrounding padding should be adjusted to reflect that the FilledButton
already contains some padding for the touch target.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I can tell setting fixedSize
would have the same visual effect as setting minimumSize
, the logic in _ButtonStyleState.build shows it as being the final decision on the constraints of the button size (as does poking around the inspector and tweaking the values there). On reflection, it would be safer to set a minimum instead in case the button contents expand beyond the button, allowing it to grow vertically. In any case, switched to mininumSize
and reduced the containing vertical padding by a total of 10px.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I started from just an empirical observation: this line with fixedSize
had no effect. If I commented it out, nothing changed. Then the further empirical observation is that the inspector shows the height of the visible button area as 40px, which is more than 38px.
OTOH when I tried setting minimumSize
here to a small value, that did have an effect — the button got shorter. So minimumSize
does have an effect where fixedSize
doesn't, even if the code that implements that is a bit tangly.
Looking at the linked code, we have:
final Size? resolvedMinimumSize = resolve<Size?>((ButtonStyle? style) => style?.minimumSize);
final Size? resolvedFixedSize = resolve<Size?>((ButtonStyle? style) => style?.fixedSize);
// …
BoxConstraints effectiveConstraints = resolvedVisualDensity.effectiveConstraints(
BoxConstraints(
minWidth: resolvedMinimumSize!.width,
minHeight: resolvedMinimumSize.height,
maxWidth: resolvedMaximumSize!.width,
maxHeight: resolvedMaximumSize.height,
),
);
if (resolvedFixedSize != null) {
final Size size = effectiveConstraints.constrain(resolvedFixedSize);
// … width …
if (size.height.isFinite) {
effectiveConstraints = effectiveConstraints.copyWith(
minHeight: size.height,
maxHeight: size.height,
);
}
}
So minimumSize
— after resolving a value for it using the defaults, which here means 40px height — feeds into effectiveConstraints
, and then that meets fixedSize
in this line:
final Size size = effectiveConstraints.constrain(resolvedFixedSize);
When they disagree, then, which one wins? The answer is up to that BoxConstraints.constrain
method. Its docs say:
Returns the size that both satisfies the constraints and is as close as possible to the given size.
So the resulting size will satisfy the constraints; it may or may not agree with the size, and will only be as close to the given size as the constraints allow. The constraints win any disagreement.
Here, that means effectiveConstraints
, and through it minimumSize
, prevail over fixedSize
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see now, and had a closer inspection of the code and BoxContraints.constrain
and stand (massively) corrected. I must have only tested with sizes > 40 as well? I did see a change as I adjusted the value but perhaps I didn't test with smaller values.
0cd3809
to
efe3708
Compare
@gnprice good to go again! |
lib/widgets/message_list.dart
Outdated
// https://www.figma.com/file/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=132-9684 | ||
// See discussion about design at: | ||
// https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/flutter.3A.20unread.20marker/near/1658008 | ||
static Color color = const HSLColor.fromAHSL(1, 227, 0.78, 0.59).toColor(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static Color color = const HSLColor.fromAHSL(1, 227, 0.78, 0.59).toColor(); | |
static final color = const HSLColor.fromAHSL(1, 227, 0.78, 0.59).toColor(); |
That way it's clear this is meant to function as a constant, and not to ever get mutated.
padding: const EdgeInsets.all(10), | ||
textStyle: const TextStyle(fontSize: 18, fontWeight: FontWeight.w200), | ||
shape: RoundedRectangleBorder(borderRadius: BorderRadius.circular(5)), | ||
backgroundColor: _UnreadMarker.color, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
msglist [nfc]: Factor out color for _UnreadMarker and MarkAsReadWidget
This change isn't NFC, because it changes the color of MarkAsReadWidget.
Could have an NFC commit without this line, just pulling out the static _UnreadMarker.color
, and then a second commit to use it here. Or this commit is fine as is, it just isn't NFC (and what it's doing isn't "factoring out" from MarkAsReadWidget, because this color wasn't previously there) — a good commit message would be msglist: Color MarkAsReadWidget the same as _UnreadMarker
.
lib/widgets/message_list.dart
Outdated
textStyle: const TextStyle(fontSize: 18, fontWeight: FontWeight.w200), | ||
shape: RoundedRectangleBorder(borderRadius: BorderRadius.circular(5)), | ||
backgroundColor: const HSLColor.fromAHSL(1,227,0.78,0.59).toColor(), | ||
fixedSize: const Size.fromHeight(38), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I started from just an empirical observation: this line with fixedSize
had no effect. If I commented it out, nothing changed. Then the further empirical observation is that the inspector shows the height of the visible button area as 40px, which is more than 38px.
OTOH when I tried setting minimumSize
here to a small value, that did have an effect — the button got shorter. So minimumSize
does have an effect where fixedSize
doesn't, even if the code that implements that is a bit tangly.
Looking at the linked code, we have:
final Size? resolvedMinimumSize = resolve<Size?>((ButtonStyle? style) => style?.minimumSize);
final Size? resolvedFixedSize = resolve<Size?>((ButtonStyle? style) => style?.fixedSize);
// …
BoxConstraints effectiveConstraints = resolvedVisualDensity.effectiveConstraints(
BoxConstraints(
minWidth: resolvedMinimumSize!.width,
minHeight: resolvedMinimumSize.height,
maxWidth: resolvedMaximumSize!.width,
maxHeight: resolvedMaximumSize.height,
),
);
if (resolvedFixedSize != null) {
final Size size = effectiveConstraints.constrain(resolvedFixedSize);
// … width …
if (size.height.isFinite) {
effectiveConstraints = effectiveConstraints.copyWith(
minHeight: size.height,
maxHeight: size.height,
);
}
}
So minimumSize
— after resolving a value for it using the defaults, which here means 40px height — feeds into effectiveConstraints
, and then that meets fixedSize
in this line:
final Size size = effectiveConstraints.constrain(resolvedFixedSize);
When they disagree, then, which one wins? The answer is up to that BoxConstraints.constrain
method. Its docs say:
Returns the size that both satisfies the constraints and is as close as possible to the given size.
So the resulting size will satisfy the constraints; it may or may not agree with the size, and will only be as close to the given size as the constraints allow. The constraints win any disagreement.
Here, that means effectiveConstraints
, and through it minimumSize
, prevail over fixedSize
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @sirpengi! Just a few nits, below and above. Also a reply above about the interaction between minimumSize
and fixedSize
on these Material button widgets, though that doesn't come with any further action items since you've already switched to minimumSize
.
lib/widgets/message_list.dart
Outdated
@@ -392,12 +393,18 @@ class MarkAsReadWidget extends StatelessWidget { | |||
// TODO(#368): this should pull from stream color | |||
color: Colors.transparent, | |||
child: Padding( | |||
padding: const EdgeInsets.all(10), | |||
// vertical padding adjusted for tap target height (48px) of containing button |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
// vertical padding adjusted for tap target height (48px) of containing button | |
// vertical padding adjusted for tap target height (48px) of button |
From the perspective of this Padding
, the button isn't containing, but rather contained — it's the child of the padding.
efe3708
to
4974433
Compare
(I see you pushed another revision — is this meant to be ready for re-review?) |
@gnprice yes, this is ready for review! (I usually wait for checks to pass and then forgot about it!) |
Use Source Sans 3 for font family and bolder weight. Readjust containing padding due to tap target size of button being larger than visible height of button.
Cool. Thanks for the revision — looks good, merging. |
4974433
to
45f223c
Compare
Thanks @chrisbobbe for the report: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/flutter.3A.20Mark-as-read/near/1679769
New screenshot: